feat: add GPR Information to UDB #1150
Conversation
Implements General Purpose Register (GPR) support in the Unified Database as requested in issue riscv-software-src#1085. Changes: - spec/schemas/register_schema.json: New schema defining structure for register files - spec/schemas/schema_defs.json: Add register-specific schema definition - spec/std/isa/register/gpr.yaml: New register file implementing all 32 RISC-V general purpose registers with proper ABI mnemonics, calling convention roles, and conditional support for RV32E - spec/std/isa/README.adoc: Update architecture documentation to include register files - tools/ruby-gems/udb/lib/udb/obj/register_file.rb: New RegisterFile class extending TopLevelDatabaseObject - tools/ruby-gems/udb/lib/udb/obj/database_obj.rb: Add RegisterFile kind to DatabaseObject::Kind enum for type system integration - tools/ruby-gems/udb/lib/udb/architecture.rb: Add register file loading support to architecture The implementation follows RISC-V ABI specifications and provides a single source of truth for GPR information. Resolves Issue: riscv-software-src#1085
AnimeshAgarwal28
requested review from
ThinkOpenly and
dhower-qc
as code owners
AnimeshAgarwal28
changed the title
AnimeshAgarwal28
changed the title
There was a problem hiding this comment.
This is, frankly, pretty awesome! Great work, @AnimeshAgarwal28 !
There are a few comments that need discussion, but this is an impressive start!
There was a problem hiding this comment.
Excellent first draft!
In addition to the inline comments, a few high-level thoughts:
To really get this integrated, we'll want to reflect it in a few places where the X registers are built in concepts. A few that come to mind: the IDL symbol table and the generated ISS.
In the symbol table, you'll find this:
riscv-unified-db/tools/ruby-gems/idlc/lib/idlc/symbol_table.rb
Lines 221 to 226 in 5d6c397
That's where "X" gets added in global scope as an array of Bits. Instead of that, we'll want to construct (all of) the register files based on the YAML. Hopefully you can get some ideas of to do so looking around here where the symbol table is instantiated:
riscv-unified-db/tools/ruby-gems/udb/lib/udb/cfg_arch.rb
Lines 462 to 488 in 5d6c397
In the ISS, the X registers are defined on a "hart" object, as you can see here:
riscv-unified-db/backends/cpp_hart_gen/templates/hart.hxx.erb
Lines 378 to 415 in 5d6c397
riscv-unified-db/backends/cpp_hart_gen/templates/hart.hxx.erb
Lines 529 to 534 in 5d6c397
We'll want to think this one through a bit more with @henrikg-qc
| when: | ||
| not: { name: E } |
There was a problem hiding this comment.
After #891, this would be:
when:
not:
extension:
name: E
Changes: - rename schema from register_schema.json to register_file_schema.json, remove "$ref": "#/$defs/register_file" from bottom of the schema, and introduce register_file_name. - drop per-register length and index fields, make abi mnemonics an array, and add 'caller_saved'/'callee_saved' booleans with default value of 'false'. - remove the 'count' helper in favor of conditioning individual register entries directly and infer indices from position. - fix the description for XLEN behavior and remove empty role arrays. - update the register-file section of README.adoc so the example mirrors the new schema. - register_file.rb: expose register data via '#data' and return Sorbet enum values for roles. Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com>
spec/schemas/schema_defs.json
Outdated
| }, | ||
| "register_file_name": { | ||
| "type": "string", | ||
| "pattern": "^[A-Za-z][A-Za-z0-9_.-]*$", |
There was a problem hiding this comment.
Let's restrict this to a single character. Since we already have X and f, either case:
| "pattern": "^[A-Za-z][A-Za-z0-9_.-]*$", | |
| "pattern": "^[A-Za-z]$", |
There was a problem hiding this comment.
Agree. And perhaps to take it a step further: make them all single letter uppercase? Right now, we have the unfortunate situation that X is uppercase but f and v are lowercase. That's out of necessity since we declare f and v as generic globals right now, and uppercase variable names are const by definition in IDL. X is special since it's builtin.
By having dedicated register file definitions, we can avoid the discrepancy.
There was a problem hiding this comment.
So, further constrain this pattern to single-character upper case, and we'll need to change the current f and v register files to F and V in the YAML and in the Ruby code?
|
Interesting... the CI is complaining:
I'm not sure exactly who's complaining about that, but I guess we should rename the file to |
…tern Changes: - Add 'writable' boolean property to register schema to indicate if a register is writable. - Update gpr.yaml to X.yaml, register directory to register_file. - Restrict regex for register_file_name to a single character. - Update README.adoc. Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1150 +/- ##
=======================================
Coverage 46.05% 46.05%
=======================================
Files 11 11
Lines 4942 4942
Branches 1345 1345
=======================================
Hits 2276 2276
Misses 2666 2666
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CI detected a couple of lines with trailing whitespace. |
We enforce the invariant that any database file must match the corresponding |
| - name: x5 | ||
| abi_mnemonics: [t0] | ||
| caller_saved: true | ||
| roles: [temporary] |
There was a problem hiding this comment.
Isn't x5 sometimes a return address?
There was a problem hiding this comment.
The only reference I can find relates to the use of x5 with shadow stack operations. Is that what you are referring to?
There was a problem hiding this comment.
No. X5 is also called the "alternate link register", and is used for calling some functions where they don't want to trash ra (millicode, and outlining).
There was a problem hiding this comment.
I guess part of the reason this is an alternate link register is because it is a temporary register, and functions that use x5 this way know it when they are constructed.
X5 does have specific support in Zicfiss for being used as a return address.
There was a problem hiding this comment.
X5 is also called the "alternate link register"
Called by whom? I can't find it in the RISC-V APIs Specification (Version 1.0: Ratified), nor in the RISC-V Assembly Programmer's Manual (2025-02-05).
I'm just wondering if this is an ad-hoc classification, since it doesn't seem to be officially documented as anything other than a temporary register.
4 participants
This PR implements GPR definition in the Unified Database, addressing issue #1085.
Problem
The UDB currently lacks information about General Purpose Registers in YAML format.
Solution
This PR adds structured register file support to UDB, starting with RISC-V general purpose registers as a foundation for future register file additions.
Changes
New Files
spec/schemas/register_schema.json: JSON schema defining the structure for YAML register file.spec/std/isa/register/gpr.yaml: Complete RISC-V general purpose register file with all 32 registers, proper ABI mnemonics, calling convention roles, and conditional support for RV32E (16 registers)tools/ruby-gems/udb/lib/udb/obj/register_file.rb: RegisterFile class extending TopLevelDatabaseObject for programmatic access to register file dataModified Files
spec/schemas/schema_defs.json: Added register-specific schema definitionsspec/std/isa/README.adoc: Updated architecture documentation to include register files alongside extensions/instructions/CSRs with usage examplestools/ruby-gems/udb/lib/udb/obj/database_obj.rb: Added RegisterFile kind to DatabaseObject::Kind enum for type system integrationtools/ruby-gems/udb/lib/udb/architecture.rb: Added register file loading support to architecture systemRegister Details
The GPR implementation includes:
Benefits
Future Work
This establishes the foundation for adding other register files mentioned in issue #1085:
Closes #1085